-
Notifications
You must be signed in to change notification settings - Fork 58
feat: improve error handling #715
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
msluszniak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any testing for this one other than sanity manual testing?
Not yet 👎🏻 |
Co-authored-by: Mateusz Sluszniak <[email protected]>
34d01b3 to
dcce471
Compare
packages/react-native-executorch/common/rnexecutorch/models/ocr/Detector.cpp
Outdated
Show resolved
Hide resolved
|
Because you've been rebasing, some files are still using std::runtime_error (DetectorUtils.cpp, Kokoro.cpp, etc.) please, try to cover those cases as well. Same thing applies for Typescript - useTextToSpeech.ts, TextToSpeechModule.ts, OCRController.ts ... - are throwing "new Error" instead of RnExecutorchError. |
…r/Detector.cpp Co-authored-by: Bartek <[email protected]>
|
I think we need to do something with this iOS CI since now majority of jobs fails with this strange error. And in fact it is not deterministic, sometimes it works perfectly fine. |
benITo47
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good to me!
Description
Previously, errors were thrown as plain strings or unstructured exceptions, making it impossible for users to programmatically handle different error types. This PR introduces type-safe, structured error handling across the entire codebase.
Changes
1. Codegen Approach
scripts/errors.config.tsErrorCodes.h) and TypeScript (ErrorCodes.ts) enumsyarn codegen:errorsto regenerate after modifying error definitions2. Error Structure
All errors now follow a consistent format:
{code: number, message: string}C++ side:
RnExecutorchError(ErrorCode, "message")instead ofstd::runtime_errorRnExecutorchInternalError::ErrorNamefor internal errorsBaseModel.cpp,LLM.cpp,Detector.cpp,Recognizer.cpp,VerticalOCR.cpp, etc.TypeScript side:
ExecutorchError(ETErrorCode.ErrorName, "message")instead of plainErrorparseUnknownError(err)in catch blocks to convert unknown errors3. Error Code Ranges
Currently that's used pretty frivolously and this needs to be fixed.
4. Key Quirk: Different Enums for C++ vs TypeScript
RnExecutorchInternalError): Only includes internal errors, excludes ExecuTorch runtime errorsETErrorCode): Includes all errors (internal + ExecuTorch mapped)ErrorVariant = std::variant<RnExecutorchInternalError, executorch::runtime::Error>5. Documentation
Added error handling documentation at
docs/docs/03-typescript-api/04-error-handling.md:Important: From Now On
When throwing errors from the native side, always use
RnExecutorchErrorwith appropriate error codes fromRnExecutorchInternalErrorenum. This ensures errors are properly serialized across the JSI boundary and can be handled by users.Introduces a breaking change?
Type of change
Tested on
Testing instructions
Screenshots
Related issues
Checklist
Additional notes